🐛 Add bundle-version and package-name annotations to CER phase objects#2580
Conversation
When upgrading between bundle versions that produce identical Kubernetes manifests, the installed version status was not updated because CER phases were identical across versions, causing in-place patches instead of new revision creation. Propagate bundle-version and package-name annotations onto each rendered object within CER phases so that different bundle versions always produce distinct phases, triggering new revision creation via phase immutability. As a side benefit, every applied bundle resource now carries two annotations that immediately tell an observer which package and version it belongs to. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Propagates bundle metadata (package name + bundle version) onto each rendered object in ClusterExtensionRevision (CER) phases so that otherwise-identical manifests across bundle versions still produce distinct phases and trigger new revision creation (fixing installed version status not updating on upgrade).
Changes:
- Add a new test bundle version (
1.0.4) that reuses the exact same bundle image as1.0.0to reproduce the “identical manifests across versions” upgrade case. - Add an E2E update scenario that upgrades from
1.0.0to1.0.4. - Update revision generation to stamp
olm.operatorframework.io/bundle-versionandolm.operatorframework.io/package-nameas annotations on each rendered object, with unit test updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| testdata/images/catalogs/test-catalog/v1/configs/catalog.yaml | Adds a new bundle version (1.0.4) that points at the same image as 1.0.0 for regression testing identical renders across versions. |
| test/e2e/features/update.feature | Adds an E2E scenario upgrading to 1.0.4 to validate upgrade behavior when bundle content is identical. |
| internal/operator-controller/applier/boxcutter_test.go | Updates unit test expectations to include the new per-object annotations. |
| internal/operator-controller/applier/boxcutter.go | Implements propagation of bundle-version/package-name annotations onto each rendered object (Helm and non-Helm paths). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2580 +/- ##
==========================================
+ Coverage 67.74% 67.84% +0.10%
==========================================
Files 137 137
Lines 9560 9591 +31
==========================================
+ Hits 6476 6507 +31
Misses 2585 2585
Partials 499 499
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
cc @grokspawn Should we be officially plumbing "release" everywhere that we plumb "version" now? |
|
I think ideally we would think in terms of CompositeVersion. I'll take a look later. |
- Rename mergeLabelMaps to mergeStringMaps since it handles both labels and annotations - Only set bundle-version/package-name annotations when values are non-empty - Update unit test to use realistic revisionAnnotations values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
left a comment
There was a problem hiding this comment.
I am OK with the proposal.
/lgtm
|
/hold |
|
/approve |
joelanford
left a comment
There was a problem hiding this comment.
Quick review of the implementation. All looks good to me except the one comment about putting Version and Release together in the BundleMetadata API/struct.
Will try to find time to review tests today if you need me to.
Can you review the Copilot reviews and address/resolve?
| type RevisionMetadata struct { | ||
| RevisionName string | ||
| Package string | ||
| Release string |
There was a problem hiding this comment.
Do we need to put this in the BundleMetadata? Seems like it is something that is a very close sibling of Version.
There was a problem hiding this comment.
Do we need to put this in the BundleMetadata? Seems like it is something that is a very close sibling of
Version.
My understanding from
#2580 (comment)
is that we want to have it, so that we can propagate it on each of applied resources. If not, we can remove it.
There was a problem hiding this comment.
I left a similar comment in my review. I feel that we will need to have another pass through this flow when we have consolidated the bundle version types down to a single one. At that point, I'd prefer to see that type consistently used with the exception of the clusterextension API itself (where we probably cannot adjust since it's GA).
But I didn't think we had to have it now. Once we merge operator-framework/operator-registry#1938 I think that we can adjust those types.
There was a problem hiding this comment.
Do we need to put this in the BundleMetadata? Seems like it is something that is a very close sibling of
Version.
After double checking not sure if we can - if add it to BundleMetadata this will also API/CRD change and that part of API is already GA. Let's keep it where it is for now until operator-framework/operator-registry#1938 merged or we address it in that PR.
There was a problem hiding this comment.
Perhaps we should hold off on release plumbing in this PR then? And then @grokspawn will cover "plumb release everywhere that version already is plumbed" as its own effort?
|
|
||
| type Release []bsemver.PRVersion | ||
|
|
||
| // String returns the dot-separated string representation of the release segments. |
There was a problem hiding this comment.
We'll be pivoting away from this type to an equivalent type in op-reg, but they behave exactly the same so we don't have to do it right this minute.
| } | ||
| } | ||
|
|
||
| func TestRelease_String(t *testing.T) { |
There was a problem hiding this comment.
I initially was going to ask for add'l testing to align this properly with its origin, but this can come when we adopt the op-reg type.
| Image: resolvedBundle.Image, | ||
| // TODO: Right now, operator-controller only supports registry+v1 bundles and has no concept | ||
| // of a "release" field. If/when we add a release field concept or a new bundle format | ||
| // we need to re-evaluate use of `AsLegacyRegistryV1Version` so that we avoid propagating |
There was a problem hiding this comment.
I think we need to have a larger conversation about the callflow here, but I think it's probably better to wait until we merge operator-framework/operator-registry#1938 and get the type adoption changes made. I was initially going to ask for changes here, but since this is more mature than that I think we can defer.
|
@joelanford I'm good with this effort. I'll let you release the hold when you have completed review. |
e18ab88 to
b18a28b
Compare
|
@grokspawn @joelanford revert the last commit, ptal |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
d57c077
into
operator-framework:main
Description
When upgrading between bundle versions that produce identical Kubernetes
manifests, the installed version status was not updated because CER phases
were identical across versions, causing in-place patches instead of new
revision creation.
Propagate bundle-version and package-name annotations onto each rendered
object within CER phases so that different bundle versions always produce
distinct phases, triggering new revision creation via phase immutability.
As a side benefit, every applied bundle resource now carries two annotations
that immediately tell an observer which package and version it belongs to.
Changes
bundle-versionandpackage-nameannotations onto each rendered object within CER phases1.0.4)mergeLabelMapstomergeStringMaps(now used for both labels and annotations)Reviewer Checklist